Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify recurring contribution template inheritance #17178

Merged
merged 1 commit into from
May 26, 2020

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Apr 27, 2020

Overview

Template CRM/Contribute/Page/ContributionRecur.tpl is called as an intermediary for CRM/Contribute/Page/ContributionRecurSelector.tpl when we can just call the latter directly.

Before

Chain of templates.

After

Just call it directly and get rid of a big IF in the template.

Technical Details

Comments

Most of the change is just whitespace from removing an "if" from the template

Testing

Make sure that you can still see the list of recurring contributions in the contact summary. And make sure that you can see all the detail when you view a recurring contribution.

@civibot
Copy link

civibot bot commented Apr 27, 2020

(Standard links)

@mattwire mattwire force-pushed the simplifyrecurtemplates branch from 97eaf68 to ecfc232 Compare April 29, 2020 10:51
@mattwire mattwire changed the title Pending #17177 Simplify recurring contribution template inheritance Simplify recurring contribution template inheritance Apr 29, 2020
@BettyDolfing
Copy link

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
      • COMMENT: We still don't know what the problem is, except something with a template. We tested it and everything still seems to work.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We created a contribution page for sign up of recurring contributions. We used the dummy payment processor for this. We then signed up for a recurring contribution and we checked the contact summary screen and then the section for contributions --> recurring contributions. We pressed view and everything worked as expected.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change potentially affects compatibility, but the risks have been sufficiently managed.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@eileenmcnaughton can you merge this PR?

@BettyDolfing
Copy link

The failing test has nothing to do with this PR. So I would assume this could be safely merged.

@eileenmcnaughton
Copy link
Contributor

OK - the fail does seem unrelated

We should use the [REF] in the PR title to make it easier for reviewers to see that there should be code maintenance improvements but no actual change in a PR (this was originally [NFC] but then that became only used for comments & spelling due to Tim challenging the NFC-ness of some of them

@eileenmcnaughton eileenmcnaughton merged commit 6f83faa into civicrm:master May 26, 2020
@eileenmcnaughton
Copy link
Contributor

@mattwire @jaapjansma looks like this caused a regression - the enable / disable buttons broke as a result of the very first line in this change

https://lab.civicrm.org/dev/core/-/issues/1961

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants